Forgot to add this here: php/frankenphp#1757

8.5 is the right branch for this? I would somehow aim to separate the cli things to only sapi/cli (for example, installing cli header from the sapi/embed sounds a bit strange) and sapi/embed to only embed. I'm not that familiar with the PHP C API to suggest something useful here but current changes are not that optimal yet, IMHO.

Or perhaps moving these common C files (objects later on in the build phase) into the php-src/main and those being used by both sapi/cli and sapi/embed? That I think would sound a bit more logical from what is the aim to achieve here if I understand everything here correctly.

8.5 is the right branch for this?

I targeted master first, but it would be nice if we could get this into php 8.5 (or earlier, 8.3 in support window?).

I would somehow aim to separate the cli things to only sapi/cli (for example, installing cli header from the sapi/embed sounds a bit strange) and sapi/embed to only embed. I'm not that familiar with the PHP C API to suggest something useful here but current changes are not that optimal yet, IMHO.

I agree, but it's kind of what is happening here. Building embed now also builds cli (minus main function, which wouldn't be in the include api anyhow). That being said, the API doesn't need to install cli headers at all, including php_embed.h is enough.

Check for example, main/fastcgi.c and its header main/fastcgi.h. This is otherwise very complex approach and quite messy but this I think should be done here instead. In that case, bulding fastcgi object is relevant to only sapi/cgi and sapi/fpm (meaning the code is shared between the two SAPIs).

Thanks, I'll have a look. It will definitely be a larger refactor though.

Because also for another reason this would need to be refactored. These sapi/cli files assume that checks like HAVE_SETPROCTITLE, HAVE_PS_STRINGS, HAVE_SYS_PSTAT_H have been performed (for example used in the sapi/cli/ps_title.c). And without building sapi/cli they wouldn't be performed at the current build system code.

In worst case scenario, it would be simplest probably to even copy&paste these CLI related files to sapi/embed and duplicate them there instead. But from this PR I see there are a lot of these CLI related files involved... And their headers then also. So, yes. I think that making some core-like part would make the most sense here (meaning going into the main directory or something).

Alternatively, we could make this a part that's only available when built when --enable-cli is used. It would make the whole thing very simple at least and we wouldn't be installing cli sapi headers from the embed build.

What do you think? If not, I'll move the shared logic (basically everything in cli sapi) into a shared folder and cli sapi becomes nothing more but a main function calling do_php_cli. But I kind of wanted to avoid large diffs because it'll make it harder to maintain it across other incoming pull requests that were based on current 8.5/master.

a part that's only available when built when --enable-cli is used

To me this would sound like different libphp.{so,a} would be produced when building with cli SAPI and without making it difficult to consume and understand why some parts sometimes are available and some aren't. No, I think that then it is better to use sapi/cli sources like in the PR. I still think providing these parts of cli sapi should be done somehow more globally like in the main directory.

Does anyone else have any suggestions here?

I targeted master first, but it would be nice if we could get this into php 8.5 (or earlier, 8.3 in support window?).

This is not a bugfix and as such is inappropriate everywhere except master.

I targeted master first, but it would be nice if we could get this into php 8.5 (or earlier, 8.3 in support window?).

This is not a bugfix and as such is inappropriate everywhere except master.

That's fair, I'll rewrite the PR on master over the weekend.

I still think providing these parts of cli sapi should be done somehow more globally like in the main directory.

Yes, if nobody comes up with a different idea, that's the direction I'll take. Will check out FastCGI sharing when I get to it. It's too late in the day now.

I checked out how the FastCGI stuff is shared, but I don't think that's a good option here. It's not just a single file that'd be shared here, it's all the feature checks and essentially all of its handling. It would just result in moving half the sapi/cli folder to main/(cli).

Huh, sorry for (automatically) adding everyone, that wasn't my intention.

That happens when rebasing before changing the target branch on GitHub. Since after the rebase you are effectively attempting to merge master into 8.5, you will ping everyone as a reviewer due to CODEOWNERS.

Huh, sorry for (automatically) adding everyone, that wasn't my intention.

That happens when rebasing before changing the target branch on GitHub. Since after the rebase you are effectively attempting to merge master into 8.5, you will ping everyone as a reviewer due to CODEOWNERS.

Thanks, I remember something similar happening before, but I forgot about it. I'll try to do better next time.

Thank you for removing the review requests.

@petk I've created another, slightly altered version of this. I also have two working versions with the other approaches, but the first takes too much fiddling on the "users" side for me to consider them a good idea, while the second moves all cli sources except php_cli_main.c into main/ and makes them general configure checks/build files.

Rebase needed @henderkes
It'd be great to get some attention on this to have it merged please 🙏

I agree, I'm kind of waiting for someone who will look at it to do the rebase. In the end it's really just moving files, but any time I rebase, it takes a day until another rebase is needed. That's why I initially preferred the solution without moving the files from sapi/cli/ into main/.

Still has no merge conflicts: #21385

Why do you need to move everything including the web server? You should move only the bits that are needed by FrankenPHP.

The web server is indeed useless for FrankenPHP, but it can be useful in other contexts and it also ensure easier backward compatibility with scripts and the like they may use it.

I don't see how it could be useful. I just don't think everything from cli should move and be always compiled. Please just strip it to minimum that you need in the same way as we did it for fastcgi.c .

I actually did move only the required pieces without process title and built-in webserver at first, but after some thought decided to pull in everything instead:

It's just not a great idea to claim frankenphp php-cli as a replacement for php when it isn't a full replacement. If a user currently uses the built in webserver for a http based queue runner, leaving it out would break that. If a user currently relies on setting the process title, leaving ps title out would break that.

It's also about 1:1 testability. By leaving the entire code in unchanged, we dodge having to fight bugs specific to our cli implementation as we currently see. There are at least a dozen of unexplained runtime issues by us using the embed sapi as a fake cli handler at the moment.